Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(24.04): add iptables, sudo and add mutation script for pam-auth-update #306

Merged
merged 18 commits into from
Dec 19, 2024

Conversation

Meulengracht
Copy link
Member

@Meulengracht Meulengracht commented Aug 2, 2024

Proposed changes

Iptables is something we include in Ubuntu Core. Unfortunately to test iptables I need both sudo (which I then figured wasn't working due to bad libpam support), which resulted in a large mutation script that emulates what pam-auth-update does.

There are dedicated integration tests for the libpam generation and sudo, and the test for iptables currently is pretty shallow since it needs kernel modules loaded that aren't available.

Checklist

Copy link

github-actions bot commented Aug 2, 2024

Diff of dependencies:

slices/libpam-runtime.yaml
@@ -1,4 +0,0 @@
-cdebconf
-debconf
-debconf-2.0
-libpam-modules

@Meulengracht Meulengracht marked this pull request as ready for review August 13, 2024 13:48
@Meulengracht Meulengracht force-pushed the slices/iptables branch 2 times, most recently from e6fa58c to 1f3198b Compare August 20, 2024 13:11
@cjdcordeiro
Copy link
Collaborator

@Meulengracht following up on this - it'd be nice to try these tests with LXD as well, once we get sort out why you're having those issues in #318

@Meulengracht Meulengracht force-pushed the slices/iptables branch 2 times, most recently from f8a92f6 to 01961a9 Compare August 28, 2024 11:02
@Meulengracht
Copy link
Member Author

@Meulengracht following up on this - it'd be nice to try these tests with LXD as well, once we get sort out why you're having those issues in #318

It passes on the LXD backend locally, I removed the caps and restored the tests

@Meulengracht
Copy link
Member Author

@cjdcordeiro @rebornplusplus this is ready for review

@cjdcordeiro cjdcordeiro requested review from cjdcordeiro and a team August 29, 2024 07:01
slices/iptables.yaml Outdated Show resolved Hide resolved
slices/iptables.yaml Show resolved Hide resolved
slices/libpam-runtime.yaml Show resolved Hide resolved
slices/libpam-runtime.yaml Outdated Show resolved Hide resolved
slices/sudo.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@cjdcordeiro cjdcordeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's all good to me, but I just have a final comment and need a 2nd review.

slices/libpam-runtime.yaml Outdated Show resolved Hide resolved
@cjdcordeiro cjdcordeiro requested a review from a team September 27, 2024 06:04
Copy link

@linostar linostar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just small nitpicks.

tests/spread/integration/iptables/task.yaml Outdated Show resolved Hide resolved
tests/spread/integration/sudo/task.yaml Show resolved Hide resolved
Copy link
Member

@rebornplusplus rebornplusplus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks quite nice, thank you! Impressive work on the mutation script.

I think the only thing major in my comments below is the future of pam-defaults slice. Let's reach a conclusion on that. And then, I think we can merge it.

slices/iptables.yaml Show resolved Hide resolved
slices/libpam-runtime.yaml Outdated Show resolved Hide resolved
slices/libpam-runtime.yaml Outdated Show resolved Hide resolved
slices/libpam-runtime.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@zhijie-yang zhijie-yang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice. Just checked the output of the pam-auth-update from a fresh container, and we have a set of diverged pam configs. The reason is that pam-auth-update reads the values from *-Initial for the first pam config of each kind.

320 # return the lines for a given config name, type, and position in the stack
321 sub lines_for_module_and_type
322 {                           
323         my ($profiles, $mod, $type, $modpos) = @_;
324         if ($modpos == 0 && $profiles->{$mod}{$type . '-Initial'}) {
325                 return $profiles->{$mod}{$type . '-Initial'};
326         }   
327         return $profiles->{$mod}{$type};
328 }   

slices/libpam-runtime.yaml Outdated Show resolved Hide resolved
slices/libpam-runtime.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@zhijie-yang zhijie-yang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor changes are required for the linting and tests. @cjdcordeiro

tests/spread/integration/libpam-runtime/task.yaml Outdated Show resolved Hide resolved
slices/libpam-runtime.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@zhijie-yang zhijie-yang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks for the changes!

Copy link
Collaborator

@cjdcordeiro cjdcordeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is in a good state now. It would be nice to also have a final pass from @clay-lake

Copy link

@clay-lake clay-lake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved! :) I'm a little concerned with the mutation script due its complexity. The current output seems consistent with what I expect in a fresh noble container, and the script should work in a fresh rootfs. Not now, but we might want to consider refactoring it if we need to adapt it in the future. Maybe something like this? I made some changes while reviewing the script.

@cjdcordeiro
Copy link
Collaborator

thanks @clay-lake
that sounds reasonable. Maybe you can add that as a TODO to our list of actions when opening 25.04? (we have an issue for that)

@cjdcordeiro cjdcordeiro merged commit 3801300 into canonical:ubuntu-24.04 Dec 19, 2024
14 checks passed
eunufe pushed a commit to norrisjeremy/chisel-releases that referenced this pull request Dec 30, 2024
…update (canonical#306)

---------

Co-authored-by: Cristovao Cordeiro <[email protected]>
Co-authored-by: Rafid Bin Mostofa <[email protected]>
Co-authored-by: zhijie-yang <[email protected]>

feat(24.04): add slices for gearman-job-server and dependencies

feat(24.04): add slices for gearman-job-server and dependencies

Added bins and services slice for gearman-job-server

feat(24.04): add slices for gearman-job-server and dependencies

Update format to match codebase

feat(24.04): add slices for gearman-tools and dependencies

feat(24.04): add slices for Gearman and dependencies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority Look at me first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants